-
Notifications
You must be signed in to change notification settings - Fork 8.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Enterprise Search] Move LicenseContext to Kea #78231
Conversation
- I noticed my IDE complaining that we were using LicensingPluginSetup (deprecated) instead of LicensingPluginStart, and decided to factor plugin.ts to DRY out / ensure all the dependencies we were passing on app mount were start services and not setup - The number of args we were passing to renderApp was getting a little ridiculous, so I created small helpers to group them up by type (Kibana's args (dependencies/services) vs our plugin's args (data, config, etc.) + bonus remove unused CoreStart type/arg
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As always, I recommend following along by commit. As a heads up, the first commit (450c6b7) is a bit of a separate tech-debt/refactor - I noticed while working on this that the license we were using was technically deprecated and that we were supposed to calling license$ from PluginsStart, not PluginsSetup. That commit addresses that + cleans up the (getting hard to read) number of args we were passing to renderApp.
private getPluginData() { | ||
// Small helper for grouping plugin data related args together | ||
return { config: this.config, data: this.data }; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems vaguely useless right now, but as a heads up I'll be adding another this.store
to this in the near-ish future (required change in order for our main app and our header menu actions to be able to share logic).
|
||
const { renderApp } = await import('./applications'); | ||
const { EnterpriseSearch } = await import('./applications/enterprise_search'); | ||
|
||
return renderApp(EnterpriseSearch, params, coreStart, plugins, this.config, this.data); | ||
return renderApp(EnterpriseSearch, kibanaDeps, pluginData); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hopefully this is now easier to read/conceptualize than the old row of args that got away from me, haha.
@@ -17,7 +16,7 @@ import { | |||
FeatureCatalogueCategory, | |||
HomePublicPluginSetup, | |||
} from '../../../../src/plugins/home/public'; | |||
import { LicensingPluginSetup } from '../../licensing/public'; | |||
import { LicensingPluginStart } from '../../licensing/public'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra context for this change:
kibana/x-pack/plugins/licensing/public/types.ts
Lines 12 to 17 in 5f781dc
export interface LicensingPluginSetup { | |
/** | |
* Steam of licensing information {@link ILicense}. | |
* @deprecated in favour of the counterpart provided from start contract | |
*/ | |
license$: Observable<ILicense>; |
I noticed it because VSCode was showing the old license$ with a strikethrough through it because of the @deprecated. Pretty cool!
|
||
describe('LicensingLogic', () => { | ||
const mockLicense = licensingMock.createLicense(); | ||
const mockLicense$ = new BehaviorSubject(mockLicense); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL I (sorta) learned how to mock observables
EDIT: TIL how to be redundant when saying TIL
<Router history={params.history}> | ||
<App {...initialData} /> | ||
</Router> | ||
</Provider> | ||
</KibanaContext.Provider> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one more wrapper to go! 🔥
- replaces useObservable with a manual subscription that updates the license value/state - moves hasXLicense checks to selectors vs helper functions
- Add mockLicensingValues to basic kea mock - Minor comment update to mockAllValues obj that I forgot to add in a previous PR
4f3d0db
to
6fa43f9
Compare
💚 Build SucceededBuild metrics@kbn/optimizer bundle module count
async chunks size
page load bundle size
History
To update your PR or re-run it, just comment with: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is 🔥
</KibanaContext.Provider> | ||
</I18nProvider>, | ||
params.element | ||
); | ||
return () => { | ||
ReactDOM.unmountComponentAtNode(params.element); | ||
unmountLicensingLogic(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this list gets much longer I might try to think of a better solution than losting all of these inputs.
const configs = [ LicenseLogic: {<LicenseLogic props>}, HttpLogic: {<HttpLogic props}, etc ]
const unmount = Object.entries(config).map((Logic, props) => { Logic(props); return Logic.mount() })
return () => {
ReactDOM.unmountComponentAtNode(params.element);
unmounts.forEach(unmount => unmount())
}
then we'd never forget to unmount!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For sure! FWIW, it stops at 4 total (by the end of my series of PRs) but I can definitely see a case for pulling all our mounts out to a separate file/helper and returning an array of unmounts. I think 4 is just on the border to not a completely warrant a separate helper, but let me know if you disagree 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* Fix licensing to use start service + refactor - I noticed my IDE complaining that we were using LicensingPluginSetup (deprecated) instead of LicensingPluginStart, and decided to factor plugin.ts to DRY out / ensure all the dependencies we were passing on app mount were start services and not setup - The number of args we were passing to renderApp was getting a little ridiculous, so I created small helpers to group them up by type (Kibana's args (dependencies/services) vs our plugin's args (data, config, etc.) + bonus remove unused CoreStart type/arg * Add LicensingLogic + mount - replaces useObservable with a manual subscription that updates the license value/state - moves hasXLicense checks to selectors vs helper functions * Update components w/ license checks to use LicensingLogic * Update tests for components now calling LicensingLogic - Add mockLicensingValues to basic kea mock - Minor comment update to mockAllValues obj that I forgot to add in a previous PR * 🔥 Remove old LicensingContext
Summary
Follow up to #78167 - part 2 electric boogaloo of our grand adventure moving all our old React Context to Kea logic.
This PR converts our old
LicenseContext
/<LicenseProvider />
to a new shinyLicensingLogic
. I got to learn a few new things about Observables while doing, and I think our tests are more robust than before - fun times!As always, I recommend following along by commit. As a heads up, the first commit (450c6b7) is a bit of a separate tech-debt/refactor - I noticed while working on this that the license we were using was technically deprecated and that we were supposed to calling license$ from PluginsStart, not PluginsSetup. That commit addresses that + cleans up the (getting hard to read) number of args we were passing to renderApp.
QA/Regression testing
Checklist